Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[stable-2.14] Fix misrendered sections in manpage generation #81380

Merged
merged 1 commit into from
Jul 31, 2023

Conversation

mattclay
Copy link
Member

Backport of #80450

This change fixes bugs in the manpage generator that existed since it was first added.

It exposes CLI ARGUMENTS value to manpage templates.

Before this change, the code contained a typo, causing the for-loop iterate over individual characters of the 'ARGUMENTS' string rather than iterating over a tuple. A missing comma was at fault.

The updated code gets rid of the for-loop and conditionals since it seems to have been a premature complexity increase and no other things than 'ARGUMENTS' were ever added into the broken iterable.

The functional change is that arguments is now always present in the Jinja2 context, unlike being missing sometimes because of the previous design (not that it was ever present, because of the bug! sigh...)

The Jinja2 templates perform an {% if arguments %} check, letting the template engine silently ignore the missing variable. The clause was always falsy, meaning that the arguments section was not included in the manpages for at least the last 6 years. With this fix, it will be.

This patch also deduplicates calling opt_doc_list @ generate_man.

It was called late in the execution, more times than necessary. This patch makes sure it happens once by putting it at the top of the scope.

It fixes rendering library and inventory in manpages.

The corresponding Jinja2 templates have blocks wrapped with conditionals like {% if inventory %} and {% if library %} but said variables were never injected into the context, nor were they even deduced on the Python side of the generator. This means that the conditional clauses were always falsy, never showing the portions of the manpages.

The Python script has hints for how the inventory variable was to be calculated, which is confirmed through the Git paleontology efforts.

The block of code that references to the inventory bit was incorrectly checking a variable with a list of nested objects for the presence of a string which was never going to work.

This patch fixes this check by verifying the CLI flag against the correct variable containing a list of options and exposes it to the Jinja2 templates.
It also exposes the library variable in a similar way.

The block displaying other binaries in Sphinx CLI docs has been synchronized with the manpage template.
Previously, the current binary was displayed also. This patch gets rid of the unwanted trailing comma there too.

Finally, the CLI executables list in the manpage template now reuses the same variable as the RST template that doesn't need any post-processing in Jinja2.
Before, it was already used in the RST template so this patch aligns both templates to use the same logic as they got out-of-sync over time.

PR #80450.
(cherry picked from commit a84b3a4)

SUMMARY
ISSUE TYPE
  • Bugfix Pull Request
  • Docs Pull Request
  • Feature Pull Request
  • Test Pull Request
COMPONENT NAME
ADDITIONAL INFORMATION

This change fixes bugs in the manpage generator that existed since it
was first added.

It exposes CLI `ARGUMENTS` value to manpage templates.

Before this change, the code contained a typo, causing the `for`-loop
iterate over individual characters of the `'ARGUMENTS'` string rather
than iterating over a tuple. A missing comma was at fault.

The updated code gets rid of the `for`-loop and conditionals since it
seems to have been a premature complexity increase and no other things
than `'ARGUMENTS'` were ever added into the broken iterable.

The functional change is that `arguments` is now always present in the
Jinja2 context, unlike being missing sometimes because of the previous
design (not that it was ever present, because of the bug! sigh...)

The Jinja2 templates perform an `{% if arguments %}` check, letting
the template engine silently ignore the missing variable. The clause
was always falsy, meaning that the arguments section was not included
in the manpages for at least the last 6 years. With this fix, it will
be.

This patch also deduplicates calling `opt_doc_list` @ generate_man.

It was called late in the execution, more times than necessary. This
patch makes sure it happens once by putting it at the top of the scope.

It fixes rendering library and inventory in manpages.

The corresponding Jinja2 templates have blocks wrapped with
conditionals like `{% if inventory %}` and `{% if library %}` but said
variables were never injected into the context, nor were they even
deduced on the Python side of the generator. This means that the
conditional clauses were always falsy, never showing the portions of
the manpages.

The Python script has hints for how the `inventory` variable was to be
calculated, which is confirmed through the Git paleontology efforts.

The block of code that references to the `inventory` bit was
incorrectly checking a variable with a list of nested objects for the
presence of a string which was never going to work.

This patch fixes this check by verifying the CLI flag against the
correct variable containing a list of options and exposes it to the
Jinja2 templates.
It also exposes the `library` variable in a similar way.

The block displaying other binaries in Sphinx CLI docs has been
synchronized with the manpage template.
Previously, the current binary was displayed also. This patch gets rid
of the unwanted trailing comma there too.

Finally, the CLI executables list in the manpage template now reuses
the same variable as the RST template that doesn't need any
post-processing in Jinja2.
Before, it was already used in the RST template so this patch aligns
both templates to use the same logic as they got out-of-sync over time.

PR ansible#80450.
(cherry picked from commit a84b3a4)

Co-authored-by: Sviatoslav Sydorenko <webknjaz@redhat.com>
@mattclay mattclay requested a review from a team as a code owner July 31, 2023 20:11
@ansibot ansibot added feature This issue/PR relates to a feature request. test This PR relates to tests. needs_triage Needs a first human triage before being processed. backport This PR does not target the devel branch. labels Jul 31, 2023
@mattclay mattclay merged commit 63413cb into ansible:stable-2.14 Jul 31, 2023
84 checks passed
@mattclay mattclay deleted the backport-a84b3a4-stable-2.14 branch July 31, 2023 21:04
@sivel sivel removed the needs_triage Needs a first human triage before being processed. label Aug 1, 2023
@ansible ansible locked and limited conversation to collaborators Aug 28, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backport This PR does not target the devel branch. feature This issue/PR relates to a feature request. test This PR relates to tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants